Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/issue 253 add active mq hosting #273

Conversation

anoordover
Copy link
Contributor

@anoordover anoordover commented Nov 19, 2024

**Closes #253 **

PR Checklist

  • Created a feature/dev branch in your fork (vs. submitting directly from a commit on main)
  • Based off latest main branch of toolkit
  • PR doesn't include merge commits (always rebase on top of our main, if needed)
  • New integration
    • Docs are written
    • Added description of major feature to project description for NuGet package (4000 total character limit, so don't push entire description over that)
  • Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Contains NO breaking changes
  • Every new API (including internal ones) has full XML docs
  • Code follows all style conventions

Other information

I'm looking for feedback on my implementation for the ActiveMQ hosting.
Please advice me about next steps I need to make to complete this functionality.

Copy link
Member

@aaronpowell aaronpowell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a nice, solid starting point. There are some code style improvements I've put in as suggestions.

Also, can you add some integration tests in the pattern of https://github.com/CommunityToolkit/Aspire/blob/main/tests/CommunityToolkit.Aspire.Hosting.Meilisearch/AppHostTests.cs to verify that the hosting integration can start and that it will be able to be used.

Lastly, should there be some health checks in place?

@anoordover
Copy link
Contributor Author

I'm looking into the healthcheck and I don't know how I could implement this.
When port 8161 is exposed I can do a GET to a jolokia endpoint that returns a json result containing the text "good".
See https://stackoverflow.com/questions/52500782/how-to-check-if-activemq-is-working-properly

Maybe I could also try to connect to the broker using the apache.NMS nuget package but I don't know if I should add this package.

b.t.w. I would expect that the container itself would tell if it is healthy or not...

@anoordover anoordover force-pushed the feature/issue_253_AddActiveMqHosting branch from d321200 to b78ccfd Compare November 20, 2024 21:38
@aaronpowell
Copy link
Member

I'm looking into the healthcheck and I don't know how I could implement this. When port 8161 is exposed I can do a GET to a jolokia endpoint that returns a json result containing the text "good". See https://stackoverflow.com/questions/52500782/how-to-check-if-activemq-is-working-properly

Maybe I could also try to connect to the broker using the apache.NMS nuget package but I don't know if I should add this package.

b.t.w. I would expect that the container itself would tell if it is healthy or not...

The container healthy status would be part of it, if the container is running, then it's "healthy", if it's stopped then it's "unhealthy" (well, the resource is stopped so that's an implied unhealthy status).

But depending on how the app inside the container works, just having the container running may not be enough. What if the app crashes in the container but the container doesn't stop? it's "healthy" in the fact that it's running, but it's really unhealthy as you can't use the endpoint(s).

If there's a HTTP endpoint that you can probe, that should do, and there's a built-in HTTP health check feature, we use it here for example: https://github.com/CommunityToolkit/Aspire/blob/main/src/CommunityToolkit.Aspire.Hosting.Azure.StaticWebApps/SwaAppHostingExtension.cs#L55

@anoordover
Copy link
Contributor Author

anoordover commented Nov 21, 2024

I use this comment to share my ideas about the health-check for the ActiveMQ hosting and try to help my thinking proces.

  • ActiveMQ has a webconsole that is exposed locally on port 8161
  • On this port there is a so called Jolokia service avaible that you can use to query the JVM
  • As shown in http calls there is an endpoint the return a text that contains the text "good" when ActiveMQ is running
  • This endpoint is secured with the same username/password as the primary endpoint that is used to connect to ActiveMQ

My ideas/questions:

  • Should we expose this endpoint/port (8161) by default?
  • I suppose I can reuse the same username/password when exposing this port. Am I correct?
  • Should I use AfterEndpointsAllocatedEvent with builder.Eventing.Subscribe?
  • Maybe use WithHttpHealthCheck as a starting point

@anoordover
Copy link
Contributor Author

I tried to add a healthcheck using the Jolokia endpoint on another branch: https://github.com/anoordover/CommunityToolkit-Aspire/tree/feature/issue_253_AddActiveMqHosting_WithHealth
Should we proceed with this solution?

@aaronpowell
Copy link
Member

I tried to add a healthcheck using the Jolokia endpoint on another branch: https://github.com/anoordover/CommunityToolkit-Aspire/tree/feature/issue_253_AddActiveMqHosting_WithHealth Should we proceed with this solution?

Was looking at that code and thought "this looks familiar" 🤣

Yep, that looks right on how it could be done. I'd collapse a few of those methods together, since you don't really need to have something that's supporting varying parameters though.

anoordover and others added 20 commits November 22, 2024 08:02
…Host/CommunityToolkit.Aspire.Hosting.ActiveMQ.AppHost.csproj

Co-authored-by: Aaron Powell <[email protected]>
…sTransit/CommunityToolkit.Aspire.Hosting.ActiveMQ.MassTransit.csproj

Co-authored-by: Aaron Powell <[email protected]>
…viceDefaults/CommunityToolkit.Aspire.Hosting.ActiveMQ.ServiceDefaults.csproj

Co-authored-by: Aaron Powell <[email protected]>
…r for received messages, extended integrationtest to check on recieved messages
@anoordover anoordover force-pushed the feature/issue_253_AddActiveMqHosting branch from 526f1a2 to 7736cb6 Compare November 22, 2024 07:02
@anoordover
Copy link
Contributor Author

anoordover commented Nov 22, 2024

So here are my next steps in my solution:

  • rebased on latest changes from master;
  • changed the MassTransit implementation to use a Request/Response pattern so a delay in the integrationtest isn't needed anymore;
  • ported the healthcheck from the temporary branch to this branch;
  • tried to simplify the healthcheck. Questions: Should I move the extension method to another class? Should I move the constants to the top of the file?

What are your best practices about line-breaks? Can I find this somewhere in the guidelines?

@aaronpowell thanks for the review.

Copy link
Member

@aaronpowell aaronpowell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good there, I think it's ready to merge.

As I'm unfamiliar with this problem space, can you explain what (if any) the intersection between this and #278 would be? since I see MassTransit used in this PR.

@anoordover
Copy link
Contributor Author

To explain a little bit about this implementation:

  1. I wanted to create an ActiveMQ aspire hosting package;
  2. I wanted to be able to use it in MassTransit;
  3. I wanted to be able to use a connectionstring when connecting to ActiveMQ from MassTransit. To be able to use ActiveMQ like this the scheme of the connectionString being exposed should be configurable because MassTransit expect "activemq" as the scheme in the connectionString while others might use something else.

Because of this I did this:

  1. Created the ActiveMQ aspire hosting package where the "user" has the ability to configure the scheme of the connectionString being provided;
  2. Created an example project to show that the communication with ActiveMQ works using MassTransit. I used MassTransit because I was able to implement a request/response "flow" easily so that Task.Delay wasn't needed in the integrationtests of the hosting package.

In short: The implementation only facilitates using MassTransit by exposing the configuration of the scheme being used in the connectionString.
MassTransit is only being used for the integration test.

# Conflicts:
#	Directory.Packages.props
@anoordover
Copy link
Contributor Author

I tried to do a rebase on main but I didn't succeed in merging this way. I did an ordinary merge from main to my branch.

@aaronpowell aaronpowell merged commit 713c94a into CommunityToolkit:main Nov 27, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Add ActiveMQ hosting
2 participants